Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(sidebars): Add XSLT and XPATH sidebar navigation #27328

Merged
merged 3 commits into from
Jun 16, 2023
Merged

feat(sidebars): Add XSLT and XPATH sidebar navigation #27328

merged 3 commits into from
Jun 16, 2023

Conversation

bsmth
Copy link
Member

@bsmth bsmth commented Jun 14, 2023

This PR adds the {{XsltSidebar}} to pages under files/en-us/web/xpath and files/en-us/web/xslt/ which were mostly missing sidebar navigation.

Screenshots

Before

image

After

image

Related issues and pull requests:

@bsmth bsmth requested a review from a team as a code owner June 14, 2023 15:33
@bsmth bsmth requested review from hamishwillee and removed request for a team June 14, 2023 15:34
@github-actions github-actions bot added the Content:Other Any docs not covered by another "Content:" label label Jun 14, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 14, 2023 •
edited
Loading

Preview URLs (108 pages)
Flaws (4)

Note! 104 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/XSLT/Transforming_XML_with_XSLT/An_Overview
Title: An overview
Flaw count: 1

  • broken_links:
    • Can't resolve /en-US/Transforming_XML_with_XSLT/For_Further_Reading

URL: /en-US/docs/Web/XSLT/Element/stylesheet
Title: <xsl:stylesheet>
Flaw count: 1

  • broken_links:
    • Can't resolve /en-US/docs/Web/XSLT/Element/for-each-group

URL: /en-US/docs/Web/XSLT/Using_the_Mozilla_JavaScript_interface_to_XSL_Transformations
Title: Using the Mozilla JavaScript interface to XSL Transformations
Flaw count: 1

  • broken_links:
    • Can't resolve /en-US/docs/Web/API/XMLDocument/load

URL: /en-US/docs/Web/XSLT/XSLT_JS_interface_in_Gecko/JavaScript_XSLT_Bindings
Title: JavaScript/XSLT Bindings
Flaw count: 1

  • macros:
    • /en-US/docs/Web/API/XMLDocument/load redirects to /en-US/docs/Web/API/XMLDocument

(comment last updated: 2023-06-16 09:12:42)

hamishwillee
hamishwillee reviewed Jun 16, 2023
View reviewed changes
files/en-us/web/xslt/using_the_mozilla_javascript_interface_to_xsl_transformations/index.md
@@ -3,6 +3,8 @@ title: Using the Mozilla JavaScript interface to XSL Transformations
slug: Web/XSLT/Using_the_Mozilla_JavaScript_interface_to_XSL_Transformations
---

{{XsltSidebar}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the sidebar but is not "in" the sidebar. I see as a non-blocking error: https://pr27328.content.dev.mdn.mozit.cloud/en-US/docs/Web/XSLT/Using_the_Mozilla_JavaScript_interface_to_XSL_Transformations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, there's a few in this directory that are not included, I'm guessing this was a space-saving decision on the older breadcrumb-style.

hamishwillee
hamishwillee reviewed Jun 16, 2023
View reviewed changes
files/en-us/web/xslt/xslt_js_interface_in_gecko/javascript_xslt_bindings/index.md
@@ -3,6 +3,8 @@ title: JavaScript/XSLT Bindings
slug: Web/XSLT/XSLT_JS_interface_in_Gecko/JavaScript_XSLT_Bindings
---

{{XsltSidebar}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has sidebar but is not in the sidebar https://pr27328.content.dev.mdn.mozit.cloud/en-US/docs/Web/XSLT/XSLT_JS_interface_in_Gecko/JavaScript_XSLT_Bindings

hamishwillee
hamishwillee reviewed Jun 16, 2023
View reviewed changes
files/en-us/web/xpath/axes/index.md
<section id="Quick_links">
{{ListSubpagesForSidebar("/en-US/docs/Web/XPath")}}
</section>
{{XsltSidebar}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note a problem with this PR, but FYI this does not get highlighted in sidebar, but is present

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be improved in the sidebar, yes. I'm not sure why it's not highlighted, will need investigation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess because it's a top-level list item which I don't think gets highlighted by default.

hamishwillee
hamishwillee approved these changes Jun 16, 2023
View reviewed changes
Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. This is all good.

Note however that an awful lot of items have the sidebar but are not in the sidebar. I think every item should be in the sidebar. Is there an action to add them?

Feel free to merge. Just leaving this open for this discussion.

@bsmth
Copy link
Member Author

bsmth commented Jun 16, 2023

Thanks a lot, Hamish! Actually I'm not sure we should merge it just yet because the newer sidebar has a regression for the reason you mentioned:

We can include the missing docs here:

This PR is basically "pages without a sidebar nav now have one", but we should include the guide docs to make sure there's no orphans. I'll track this in a Yari issue and we can wait for the improvements to land first.

@hamishwillee
Copy link
Collaborator

I'll track this in a Yari issue and we can wait for the improvements to land first.

That is your call. I think I'd merge it as "better than what we have now".

@bsmth
Copy link
Member Author

bsmth commented Jun 16, 2023

That is your call. I think I'd merge it as "better than what we have now".

Thanks Hamish, fair enough, I will merge shortly. Getting coverage of pages without sidebars was the goal here and improving the sidebar itself should be done soon in that case. It may even cover XPATH, XSLT, EXSLT & XML, but it will need testing & feedback to make sure it's usable.

@bsmth bsmth merged commit 86c3d95 into mdn:main Jun 16, 2023
@bsmth bsmth deleted the xslt-sidebar branch June 16, 2023 09:16
@bsmth
Copy link
Member Author

bsmth commented Jun 16, 2023

Adding a follow-up task to improve the sidebar here: mdn/yari#9099

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamishwillee hamishwillee hamishwillee approved these changes

Assignees
No one assigned
Labels
Content:Other Any docs not covered by another "Content:" label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bsmth @hamishwillee